-
Notifications
You must be signed in to change notification settings - Fork 133
[WOOMOB-1516] - Booking attendance status #14778
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Generated by 🚫 Danger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Implements booking attendance status end-to-end: persistence, mapping, network update, UI display, and update-in-progress skeleton states.
- Adds attendanceStatus to BookingEntity with Room converters and DB auto-migration.
- Maps API field attendance_status to domain/UI, exposes updateAttendanceStatus via repository/store/client.
- Updates UI to display attendance status, handle update via bottom sheet, and show SkeletonView while updating.
Reviewed Changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/entity/BookingEntity.kt | Adds AttendanceStatus sealed type, field on entity, and type converters. |
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/WCAndroidDatabase.kt | Bumps DB version and adds auto-migration for new column. |
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/network/rest/wpcom/wc/bookings/BookingsRestClient.kt | Adds endpoint to update attendance status. |
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/network/rest/wpcom/wc/bookings/BookingsStore.kt | Adds flow to call REST client, map DTO, and persist updated booking. |
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/network/rest/wpcom/wc/bookings/BookingDto.kt | Adds attendance_status field to DTO. |
libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/network/rest/wpcom/wc/bookings/BookingDtoMapper.kt | Maps attendance_status to entity. |
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/BookingsRepository.kt | Exposes updateAttendanceStatus to app layer. |
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/BookingMapper.kt | Maps entity attendance to UI, threads update-in-progress state. |
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/details/* | Wires selection to repository, tracks AttendanceUpdateStatus, updates state. |
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/compose/* | Renders attendance tag/section with Skeleton during updates; updates enums → sealed types. |
WooCommerce/src/main/res/values/strings.xml | Adds error string for attendance status updates. |
WooCommerce/src/test/* | Adjusts tests for new API and verifies update call. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...ce/src/main/kotlin/com/woocommerce/android/ui/bookings/compose/BookingAttendanceStatusTag.kt
Outdated
Show resolved
Hide resolved
...ce/src/main/kotlin/com/woocommerce/android/ui/bookings/compose/BookingAttendanceStatusTag.kt
Show resolved
Hide resolved
...erce/src/main/kotlin/com/woocommerce/android/ui/bookings/compose/BookingAttendanceSection.kt
Show resolved
Hide resolved
...fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/persistence/entity/BookingEntity.kt
Outdated
Show resolved
Hide resolved
val orderResult = bookingDto.orderId.takeIf { it != 0L }?.let { | ||
orderStore.fetchSingleOrderSync(site, bookingDto.orderId) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is needed - asked here p1760707694187709/1760706907.922799-slack-CNA89KY6M
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The answer is that the Order won't change, so I can update this to not make the extra request and only update the attendance status in the DB. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree. Given this update doesn't have any impact on the associated order I'd refrain from making that extra request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done here - 3216cca
val parentId: Long, | ||
val personCounts: List<Long>?, | ||
val localTimezone: String, | ||
@ColumnInfo(defaultValue = "") val attendanceStatus: AttendanceStatus, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this, as a default okay? All new bookings start with booked
so we could do the same 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, would that mean that for bookings created prior to adding the attendance_status
change to the API, we'd be setting it to booked
?
What is wp-admin
doing for old bookings that didn't have attendance_status
set? I think we might simply want to leave it empty or null
and avoid displaying any label or text when we don't have any real value.
If a default value is to be added for old bookings, then I think it should be the backend doing so not us.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, would that mean that for bookings created prior to adding the attendance_status change to the API, we'd be setting it to booked?
Correct, that's probably not good.
What is wp-admin doing for old bookings that didn't have attendance_status set?
Good question, I will try to find it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, would that mean that for bookings created prior to adding the attendance_status change to the API, we'd be setting it to booked?
Given there are no CIAB production users, I think this should be fine 🤔
The plugin seems to be using booked
by default even for existing bookings as can be seen in L31 of class-wc-booking.php
.
But for this special case, I think not setting a default is safest, we refresh the bookings each time they are open, so it's better to avoid hardcoding a default that can be changed by the plugin anytime, and hiding the value while we fetch the new value should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think we might simply want to leave it empty or null and avoid displaying any label or text when we don't have any real value.
I've done that for now - updated in d4e83cc
BookingEntity.AttendanceStatus.Booked -> BookingAttendanceStatus.Booked | ||
BookingEntity.AttendanceStatus.CheckedIn -> BookingAttendanceStatus.CheckedIn | ||
BookingEntity.AttendanceStatus.NoShow -> BookingAttendanceStatus.NoShow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you can see, there's no Cancelled
here - not sure yet how we will determine such attendance status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to hide the AttendanceStatus when the booking is cancelled p1759237996387569/1759139627.152819-slack-C09224W4HAA
So it means there is no need for a Cancelled
attendance status 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest moving this discussion to Slack to increase visibility and reach a final decision for both platforms.
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #14778 +/- ##
============================================
- Coverage 38.06% 38.03% -0.04%
- Complexity 10020 10024 +4
============================================
Files 2125 2125
Lines 119772 119928 +156
Branches 16314 16357 +43
============================================
+ Hits 45589 45609 +20
- Misses 69586 69712 +126
- Partials 4597 4607 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
02ac4c1
to
8b2f7dd
Compare
71bf4f8
to
d4e83cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @AdamGrzybkowski 🥇
I'm pre-approving the PR as code looks good and works well. About how to deal with the cancelled state we can either wait to merge this or handle it in a different PR once we discuss and decide how to proceed. I'll leave it up to you.
I will merge and we can update later as this PR adds more than just displaying a badge. |
WOOMOB-1516
Description
This PR implements the
attendance_status
we get from the API. This value is now displayed and also update after selecting an option from the bottom sheet.For the loading progress indicator, I've decided to use the SkeletonView (see the recording below).
Only the bookings created after installing the latest plugin will have the status set. The old one will have an empty string. I didn't handle this in any way as this should be an issue for us during the development phase. But if you think we should, let me know.
Steps to reproduce
booked
status shownTesting information
Test network connection issues and response errors.
The tests that have been performed
The above
Images/gif
Screen_recording_20251017_151920.mp4
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.